Misc layout improvements in dataset upload #8231
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several user-facing changes to the WEBKNOSSOS application, including enhancements to error messages, improvements in the dataset upload process, and restructuring of UI components for better clarity. Key updates include the transition to asynchronous image file reading, removal of bounding box restrictions for super users, and various bug fixes. Additionally, the changelog has been updated to reflect these changes, and Google Analytics integration has been removed. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
CHANGELOG.unreleased.md (1)
22-22: Consider rephrasing for clarity and conciseness.The phrase "a lot of" could be replaced with a more precise term:
-Fixed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.com/scalableminds/webknossos/pull/8176) +Fixed performance bottleneck when deleting multiple trees at once. [#8176](https://github.com/scalableminds/webknossos/pull/8176)🧰 Tools
🪛 LanguageTool
[style] ~22-~22: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...(A_LOT_OF)
frontend/javascripts/components/brain_spinner.tsx (1)
81-92: LGTM! Consider refining the spacing stylesThe new layout structure using
flex-center-childwith gap spacing improves the alignment and spacing between the organization switch button and dashboard link. This aligns well with the PR's objective of enhancing UI layouts.Consider removing the
marginTopstyle from the Link component since spacing is now handled by the parent's gap property:<Link to="/" - style={{ - marginTop: 16, - }} >frontend/javascripts/admin/dataset/dataset_add_view.tsx (1)
314-334: Consider standardizing button spacing.While the button layout is good, consider using a design system constant for consistent spacing between buttons across modals.
Example enhancement:
<React.Fragment> <Button type="primary" + style={{ marginRight: 8 }} // Use your design system spacing constant onClick={() => history.push(`/datasets/${organization}/${datasetName}/view`)} > View the Dataset </Button> <Button + style={{ marginRight: 8 }} // Use your design system spacing constant onClick={() => history.push(`/datasets/${organization}/${datasetName}/edit`)} > Go to Dataset Settings </Button> <Button onClick={() => history.push("/dashboard/datasets")}> Go to Dashboard </Button> </React.Fragment>frontend/javascripts/admin/dataset/dataset_upload_view.tsx (2)
371-373: Enhance error message with more contextConsider improving the error message by including the dataset name and error details for better debugging:
if (maybeError != null) { Toast.error( - "The upload was successful, but the conversion for the dataset could not be started. Please try again or contact us if this issue occurs again.", + `The upload was successful, but the conversion for dataset "${formValues.name}" could not be started. Error: ${maybeError.message}. Please try again or contact us if this issue occurs again.` ); }
Line range hint
493-520: Move inline styles to CSS classesThe layout improvements look good, but consider moving the inline styles to CSS classes for better maintainability and reusability:
-<Spin spinning={isFinishing} style={{ marginTop: 4 }} tip="Processing uploaded files …"> +<Spin spinning={isFinishing} className="dataset-upload-spinner" tip="Processing uploaded files …"> <div - style={{ - display: "flex", - alignItems: "center", - flexDirection: "column", - }} + className="dataset-upload-content" >Add these CSS classes to your stylesheet:
.dataset-upload-spinner { margin-top: 4px; } .dataset-upload-content { display: flex; align-items: center; flex-direction: column; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/dataset/dataset_add_view.tsx(1 hunks)frontend/javascripts/admin/dataset/dataset_upload_view.tsx(3 hunks)frontend/javascripts/components/brain_spinner.tsx(1 hunks)frontend/javascripts/messages.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/messages.tsx
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~22-~22: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...
(A_LOT_OF)
🔇 Additional comments (1)
frontend/javascripts/admin/dataset/dataset_add_view.tsx (1)
309-311: LGTM! Verify the centered-items class.
The switch to using a dedicated CSS class for centering improves maintainability and consistency.
Let's verify the existence and implementation of the centered-items class:
✅ Verification successful
The centered-items class is properly implemented and safe to use
The class is defined in _utils.less with appropriate flex properties for centering items horizontally:
- Uses
display: flexwithflex-direction: row - Centers content with
justify-content: center
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the centered-items class definition and usage
rg "centered-items" --type css
rg "centered-items" --type less
rg "centered-items" --type scss
Length of output: 323
Script:
#!/bin/bash
# Let's check the implementation of centered-items class in _utils.less
cat frontend/stylesheets/_utils.less | grep -A 5 "centered-items"
# Also check if this class is imported/used in the dataset view's style dependencies
rg -l "_utils\.less" frontend/javascripts/admin/dataset/
Length of output: 220
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
22-22: Consider using more precise wording.Replace "a lot of" with a more specific phrase for better clarity.
-Fixed performance bottleneck when deleting a lot of trees at once. +Fixed performance bottleneck when deleting multiple trees simultaneously.🧰 Tools
🪛 LanguageTool
[style] ~22-~22: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...(A_LOT_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~22-~22: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...
(A_LOT_OF)
🔇 Additional comments (1)
CHANGELOG.unreleased.md (1)
27-27: Previous review comment is still applicable.
|
When testing I noticed two issues:
|
|
@hotzenklotz Thanks for catching these things! I could reproduce the progress=100 thingie when uploading another dataset after a first DS was uploaded. For the very first DS, it already worked for me. I pushed a fix for the non-first DS. For your other point, I changed the layout so that the two phases look like this now:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
27-27: Consider rephrasing for clarity and conciseness.Replace "a lot of" with a more precise phrase.
-Fixed performance bottleneck when deleting a lot of trees at once. +Fixed performance bottleneck when deleting multiple trees simultaneously.🧰 Tools
🪛 LanguageTool
[style] ~27-~27: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...(A_LOT_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/dataset/dataset_add_view.tsx(1 hunks)frontend/javascripts/admin/dataset/dataset_upload_view.tsx(4 hunks)frontend/javascripts/messages.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/messages.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/admin/dataset/dataset_add_view.tsx
- frontend/javascripts/admin/dataset/dataset_upload_view.tsx
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~27-~27: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...
(A_LOT_OF)
🔇 Additional comments (1)
CHANGELOG.unreleased.md (1)
35-35: Enhance the changelog entry with more specific details.
The current entry is too vague and should reflect the specific improvements mentioned in the PR objectives, including the upload progress bar and spinner visibility issues reported in the PR comments.
-Fixed some layout issues in the upload view. [#8231](https://github.com/scalableminds/webknossos/pull/8231)
+Fixed layout issues in the dataset upload view: removed redundant toasts, improved upload modal layout, enhanced the "dataset does not exist" screen, and fixed upload progress bar and spinner visibility issues. [#8231](https://github.com/scalableminds/webknossos/pull/8231)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
28-28: Consider using more precise language.The phrase "a lot of" is imprecise. Consider using more specific wording to better convey the impact of this fix.
-Fixed performance bottleneck when deleting a lot of trees at once. +Fixed performance bottleneck when deleting multiple trees simultaneously.🧰 Tools
🪛 LanguageTool
[style] ~28-~28: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...(A_LOT_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~28-~28: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ed performance bottleneck when deleting a lot of trees at once. [#8176](https://github.c...
(A_LOT_OF)
🔇 Additional comments (1)
CHANGELOG.unreleased.md (1)
36-36: Enhance the changelog entry with more specific details.
The current entry is too vague and doesn't capture the full scope of the improvements made in PR #8231.
-Fixed some layout issues in the upload view. [#8231](https://github.com/scalableminds/webknossos/pull/8231)
+Fixed layout issues in the dataset upload view: removed redundant toasts, improved upload modal layout, and enhanced the "dataset does not exist" screen. [#8231](https://github.com/scalableminds/webknossos/pull/8231)
The toasts were redundant because the popover showed the same information. I also fixed the layout in the upload modals (as well as one in the "this dataset does not exist" screen).
Best viewed with "Hide whitespace".
URL of deployed dev instance (used for testing):
Steps to test:
I checked the following scenarios (ignore the theme change):
before:
after:
before:
after:
Issues: